Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Redesign RegisterCallback API #3584

Merged
merged 34 commits into from
Jan 19, 2023
Merged

Redesign RegisterCallback API #3584

merged 34 commits into from
Jan 19, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jan 11, 2023 •
edited
Loading

Fix #3519
Fix #3583

Issues

As stated in #3519 and #3583, the current RegisterCallback method and related Callback have the following issues:

  • Not compliant with specification recommendations and requirements.
  • Require the user to propagate a context for valid observations.
  • Allow for the observation of unregistered instruments.
  • Allow for the observation of instruments from other Meters.

Redesign Callback

The Callback type is updated to accept a new Observer type.

type Callback func(context.Context, Observer) error

The Observer type matches what was introduced in #3507 for single instrument callbacks. It is used to make an observation for an instrument that is passed as an argument to one of its "typed" methods.

type Observer interface {
	ObserveFloat64(obsrv instrument.Float64Observer, value float64, attributes ...attribute.KeyValue)
	ObserveInt64(obsrv instrument.Int64Observer, value int64, attributes ...attribute.KeyValue)
}

This new design allows for the SDK implementation to hold a registry of instruments a Callback is registered with. When the callback is executed the SDK can then check that the observations made by a user apply to one of these registered instruments. It is able to block observations of non-SDK/non-same-Meter instruments and doesn't require the user to complete a context passing operation.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jan 11, 2023
@MrAlias MrAlias added this to the Metric v0.35.0 milestone Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 11, 2023 •
edited
Loading

Codecov Report

Merging #3584 (eec9b94) into main (f941b3a) will increase coverage by 0.1%.
The diff coverage is 92.3%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3584     +/-   ##
=======================================
+ Coverage   78.9%   79.0%   +0.1%     
=======================================
  Files        170     170             
  Lines      12460   12587    +127     
=======================================
+ Hits        9834    9950    +116     
- Misses      2417    2424      +7     
- Partials     209     213      +4
Impacted Files Coverage Δ
metric/internal/global/instruments.go 51.8% <0.0%> (-2.5%) ⬇️
sdk/metric/meter.go 86.5% <94.3%> (+7.1%) ⬆️
sdk/metric/instrument.go 95.8% <100.0%> (+2.9%) ⬆️
sdk/metric/pipeline.go 91.3% <100.0%> (ø)
metric/internal/global/meter.go 93.8% <0.0%> (-1.5%) ⬇️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

@MrAlias
Update RegisterCallback and Callback decls
14a7c30 
RegisterCallback accept variadic Asynchronous instruments instead of a
slice.

Callback accept an observation result recorder to ensure instruments
that are observed by a callback.
@MrAlias
Update global impl
4a0d32f
@MrAlias
Update noop impl
7110c78
@MrAlias
Update SDK impl
e87ea5f
@MrAlias
Fix prometheus example
68d8224
@MrAlias
Fix metric API example_test
8c88dea
@MrAlias
Remove unused registerabler
93a5d85
@MrAlias MrAlias mentioned this pull request Jan 11, 2023
Merged
@MrAlias

This comment was marked as resolved.

Sign in to view
@MrAlias MrAlias marked this pull request as ready for review January 12, 2023 00:01
@MrAlias MrAlias requested a review from jmacd as a code owner January 12, 2023 00:01
@MadVikingGod
Copy link
Contributor

There should never be a case where users use instrument.Observe after this change. Can you link where you see that?

When you use the instrument's WithCallback() you use an [Int|Float]64Observer, and in that callback you use inst.Observe(). When you use RegisterCallback() if you use an inst.Observe() that is an error, you are instead supposed to use obs.Observe[Int|Float]64(inst, ...).

This will trip new users up.

MadVikingGod
MadVikingGod reviewed Jan 18, 2023
View reviewed changes
sdk/metric/meter.go Outdated Show resolved Hide resolved
MadVikingGod
MadVikingGod reviewed Jan 18, 2023
View reviewed changes
sdk/metric/meter.go Outdated Show resolved Hide resolved
@MadVikingGod
Copy link
Contributor

The delegated instruments are still not useable, I updated the gist to the new failure point.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 18, 2023

When you use the instrument's WithCallback() you use an [Int|Float]64Observer, and in that callback you use inst.Observe(). When you use RegisterCallback() if you use an inst.Observe() that is an error, you are instead supposed to use obs.Observe[Int|Float]64(inst, ...).

If by inst.Observe() you mean a user calls Observe on an instrument directly when they observe in a callback registered at creation, this is not true. As per your suggestion in #3507 (comment), the callbacks registered at creation are also observed via a passed parameters to the callback:

opentelemetry-go/metric/instrument/asyncfloat64.go

Lines 61 to 73 in e8c6e45

// Float64Callback is a function registered with a Meter that makes
// observations for a Float64Observer it is registered with.
//
// The function needs to complete in a finite amount of time and the deadline
// of the passed context is expected to be honored.
//
// The function needs to make unique observations across all registered
// Float64Callbacks. Meaning, it should not report measurements with the same
// attributes as another Float64Callbacks also registered for the same
// instrument.
//
// The function needs to be concurrent safe.
type Float64Callback func(context.Context, Float64Observer) error

opentelemetry-go/metric/instrument/asyncint64.go

Lines 62 to 74 in e8c6e45

// Int64Callback is a function registered with a Meter that makes
// observations for an Int64Observer it is registered with.
//
// The function needs to complete in a finite amount of time and the deadline
// of the passed context is expected to be honored.
//
// The function needs to make unique observations across all registered
// Int64Callback. Meaning, it should not report measurements with the same
// attributes as another Int64Callbacks also registered for the same
// instrument.
//
// The function needs to be concurrent safe.
type Int64Callback func(context.Context, Int64Observer) error

For example:

opentelemetry-go/metric/example_test.go

Lines 58 to 74 in e8c6e45

instrument.WithInt64Callback(func(ctx context.Context, inst instrument.Int64Observer) error {
// Do the real work here to get the real disk usage. For example,
//
// usage, err := GetDiskUsage(diskID)
// if err != nil {
// if retryable(err) {
// // Retry the usage measurement.
// } else {
// return err
// }
// }
//
// For demonstration purpose, a static value is used here.
usage := 75000
inst.Observe(ctx, int64(usage), attribute.Int("disk.id", 3))
return nil
}),

MrAlias
MrAlias commented Jan 18, 2023
View reviewed changes
sdk/metric/meter_test.go Outdated Show resolved Hide resolved
@MadVikingGod MadVikingGod self-requested a review January 18, 2023 22:20
MadVikingGod
MadVikingGod reviewed Jan 18, 2023
View reviewed changes
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked that the delegates work. I've not had time to check the rest of the changes.

If this doesn't clear it, you can override the Request changes.

@MadVikingGod MadVikingGod merged commit 69b18e6 into open-telemetry:main Jan 19, 2023
@MrAlias MrAlias deleted the multi-cback-reg branch January 19, 2023 15:38
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Jan 19, 2023
@MrAlias
Remove the unused produceKey and callbackKey
f74cc7e 
Following open-telemetry#3584, this value and type are no longer used.
@MrAlias MrAlias mentioned this pull request Jan 19, 2023
Merged
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Jan 19, 2023
@MrAlias
Remove stale comments from metric global
5a379bb 
The Observe is not used to record measurements following open-telemetry#3584.
@MrAlias MrAlias mentioned this pull request Jan 19, 2023
Merged
@MadVikingGod MadVikingGod mentioned this pull request Jan 19, 2023
Merged
MrAlias added a commit that referenced this pull request Jan 19, 2023
@MrAlias
Remove stale comments from metric global (#3603)
b1a8002 
The Observe is not used to record measurements following #3584.
MrAlias added a commit that referenced this pull request Jan 20, 2023
@MrAlias
Remove the unused produceKey and callbackKey (#3602)
88f6000 
Following #3584, this value and type are no longer used.
@MrAlias MrAlias mentioned this pull request Jan 28, 2023
Merged
@MrAlias MrAlias mentioned this pull request Apr 8, 2023
Closed
@dashpole dashpole mentioned this pull request Oct 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

@MadVikingGod MadVikingGod MadVikingGod approved these changes

@jmacd jmacd Awaiting requested review from jmacd

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@pellared pellared Awaiting requested review from pellared pellared is a code owner

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

Assignees
No one assigned
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Go: Metric API (GA)
Status: Done
Go: Metric SDK (Beta)
Status: Done
Milestone
Metric v0.35.0
4 participants
@MrAlias @MadVikingGod @dashpole @hanyuancheung